Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: should ensure can access all decorators from app built with helper.build #742

Merged
merged 7 commits into from
Jul 25, 2024

Conversation

jean-michelet
Copy link
Contributor

Fixes #622

@jordanebelanger
Copy link

I really need this fix merged in 😊

@jean-michelet jean-michelet changed the title fix: should ensure can access all decorators from app build with helper.build fix: should ensure can access all decorators from app built with helper.build Jul 24, 2024
@jean-michelet
Copy link
Contributor Author

I really need this fix merged in 😊

Next time, feel free to push a PR with unit tests. We'd be happy to review your code and merge the fixes.

Comment on lines +175 to +177
const appFn = file.default || file
const appPlugin = appConfig.skipOverride ? fp(appFn) : appFn
await fastify.register(appPlugin, appConfig)
Copy link
Contributor Author

@jean-michelet jean-michelet Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it's possible to reset the fp wrapping without breaking the existing. We can allow a skipOverride option here. On the other hand, I don't know if it's a good idea to pass it in additionalOptions, if a user is using a skipOverride option that does something different in the app, it could be an unexpected side effect, right? Altough seems very unlikely.

I can avoid this with a destructuration:

const { skipOverride, ...config } = appConfig
await fastify.register(appPlugin, config)

We can also pass a fourth argument to runFastify, but it seems a bit overkill.

Any suggestion?

@jean-michelet
Copy link
Contributor Author

Ready for review @mcollina @Eomm

@jean-michelet
Copy link
Contributor Author

jean-michelet commented Jul 24, 2024

@jordanebelanger

Even if I've already pushed a PR, if the tests are broken and you have the right solution, that's OK to push a new PR or to tell me why my PR is wrong and how I can fix it.

Fastify is community driven, so maintainers don't usually solve bugs for you.

I left you this message to save you time, not to lecture you, just to be clear ^^

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 6c83cd3 into fastify:master Jul 25, 2024
14 checks passed
@jordanebelanger
Copy link

jordanebelanger commented Jul 25, 2024

@jordanebelanger

Even if I've already pushed a PR, if the tests are broken and you have the right solution, that's OK to push a new PR or to tell me why my PR is wrong and how I can fix it.

Fastify is community driven, so maintainers don't usually solve bugs for you.

I left you this message to save you time, not to lecture you, just to be clear ^^

I don't understand what you are talking about, I saw that there is a PR open for this feature/issue, and I am merely expressing my interest for this being completed, personally I love it when users express their need for specific issues being fixed to help with triage. To each their own I guess.

edit: just saw the merge, thank you very much 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't access plugin inside tests
3 participants